I'm not sure if I understand this correctly - but we currently already mark react-refresh as a peerDependency, which actually satisfies the "packages should only ever require what they formally marks as dependencies" part. I'm not sure if we fall into the part of "configuration generator" (like eslint plugins) where we should just mark react-refresh as a "defaulted-peer-dependency".

I'm quite sure that this actual change won't cause much harm in effect, but I just want to understand the situation a bit more cause I think it is actually CRA that falls into the "configuration generator" category but not us.

but we currently already mark react-refresh as a peerDependency, which actually satisfies the "packages should only ever require what they formally marks as dependencies" part

Sort of, before this PR it didn't actually require it, whichever file it was injected into did, resolving it up front like this makes sure that it uses the version provided to it through the peerDependency.

I'm quite sure that this actual change won't cause much harm in effect, but I just want to understand the situation a bit more cause I think it is actually CRA that falls into the "configuration generator" category but not us.

Imagine for a moment that there is no hoisting, that will give a layout like this:

package.json
node_modules
  react-scripts
    node_modules
      react-refresh
      @pmmmwh/react-refresh-webpack-plugin
src
  index.jsx

Now @pmmmwh/react-refresh-webpack-plugin injects require('react-refresh/runtime') into /src/index.js which webpack can't find so it fails, with this change it's injected as require('/node_modules/react-scripts/node_modules/react-refresh/runtime') which webpack can find

Hiya 👋 (note that @merceyz is a Yarn maintainer as well and has plenty of context on hoisting, dependencies, etc 😃)

I'm not sure if I understand this correctly - but we currently already mark react-refresh as a peerDependency, which actually satisfies the "packages should only ever require what they formally marks as dependencies" part.

You do, and as a result it's perfectly fine if you require react-refresh! The problem is that, in the present case, react-refresh-webpack-plugin isn't the (only?) one doing the require call. Since you're literally injecting the following into the transformed user code, from Webpack's perspective, the user code is the one that depends on react-refresh, and since it isn't listed as a dependency there, it's dangerously vulnerable to hoisting problems:

import {...} from 'react-refresh/runtime';

On PnP installs this translates into a hard error, whereas on npm installs this would silently "work", but there would be no guarantee as to the version of react-refresh that would end up being used (since the hoisting is unpredictable). It would usually work, until you add a separate workspace that would use a more recent version of react-refresh-webpack-plugin with an incompatible react-refresh, and suddenly your old application would start to crash.

The fix @merceyz made, that calls require.resolve on react-refresh, ensures that you store the fully resolved path into the transformed sources rather than the bare identifier. By doing this, you ensure that Webpack will now from the get go where to find react-refresh, and won't try to resolve it relative to the source being processed.

Thanks for the clarifications (to both of you)! I'll merge this tomorrow and hopefully push out a patch version soon.